Carry Iceberg REST TableUpdate deltas on the snapshots commit request, and use them for audit branchRefName - #669
Draft
cbb330 wants to merge 2 commits into
Draft
Conversation
Adds an optional `jsonMetadataUpdates` field to IcebergSnapshotsRequestBody holding a commit's deltas as Iceberg REST spec `TableUpdate` actions, and populates it client-side from `TableMetadata.changes()`. Why: `jsonSnapshots` and `snapshotRefs` carry complete replacement state, so the request describes what the table now looks like but never what the commit actually changed. The server recovers the deltas by diffing the incoming snapshot set against prior state (OpenHouseInternalTableOperations), and some changes cannot be recovered at all — `CREATE BRANCH b` adds a ref at the current head and commits no snapshot, so the resulting state is indistinguishable from a no-op on main. The Iceberg REST spec already models this: `CommitTableRequest.updates[]` is a list of `TableUpdate` actions, where that operation is a single `set-snapshot-ref` with `ref-name: b`, `type: branch`, and no `add-snapshot`. The client has the list for free via `TableMetadata.changes()` — the same one every REST catalog sends — and `MetadataUpdateParser` emits the spec wire format verbatim. This ships the spec shape rather than a lookalike, so when OpenHouse adopts the REST commit endpoint the field is promoted to `updates` and the full-state fields retire, with no re-modeling. Additive and advisory: the server still builds table metadata from jsonSnapshots/snapshotRefs and is not changed here, clients predating the field omit it, and consumers must tolerate null/empty. Serialization failures are swallowed and individual unserializable actions skipped, so the field can never fail a commit. One source file serves both the iceberg-1.2 and iceberg-1.5 runtimes. No consumer yet — the audit path and the server-side delta handling land separately. Tests assert `changes()` yields the expected spec actions per operation: CREATE BRANCH emits only a branch-typed `set-snapshot-ref`, CREATE TAG is tag-typed, DROP BRANCH is a `remove-snapshot-ref`, and an append reports both `add-snapshot` and the ref that moved. Note `changes()` accumulates across builds within a session, so the fixture discards construction history to match production, where the base always comes from a refresh parsed off disk.
Table operations against named Iceberg branches are not currently observable: TableAuditEvent records currentSnapshotId and currentSnapshotTimestampMs but carries no signal for which branch ref a commit wrote. Add branchRefName, populated from the `set-snapshot-ref` action in the request's jsonMetadataUpdates. The commit states which ref it moved and whether that ref is a branch or a tag, so the field reports what happened rather than inferring it. Inferring it from the resulting table state does not work. The obvious approach — match the last snapshot in jsonSnapshots to the ref pointing at it — fails on the operation this field exists to observe: `CREATE BRANCH b` creates a ref at the current head and commits no snapshot, so main and b both point at the last snapshot and the answer depends on HashMap iteration order. It would also treat tags as branches, since SnapshotRefParser parses both and ref type is absent from that comparison, and it assumes the client always serializes snapshots chronologically. None of those hold for ref-only commits. Clients predating jsonMetadataUpdates omit it; branchRefName is then left unset rather than guessed, since an absent audit field is preferable to one that is wrong roughly half the time. Unparseable actions are skipped individually so a single bad entry cannot hide the rest, and extraction stays best-effort — it cannot fail the request. currentSnapshotId and currentSnapshotTimestampMs continue to track the main ref unchanged, for backwards compatibility. This reports what the client declared, not what the server committed. Making it authoritative means having doCommit surface the refs it actually changed, which it already computes internally; that is a separate change. Tests cover CREATE BRANCH at head, determinism across ref iteration order, CREATE TAG, DROP BRANCH, clients omitting the field, and malformed actions.
cbb330
marked this pull request as draft
August 3, 2026 05:25
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds an optional
jsonMetadataUpdatesfield toIcebergSnapshotsRequestBodycarrying a commit's deltas as Iceberg REST specTableUpdateactions, and uses it to populate a newbranchRefNameonTableAuditEventso operations against named Iceberg branches become observable.Today the commit request carries only replacement state (
jsonSnapshots+snapshotRefs) — what the table now looks like, never what the commit changed. That makes "which branch was written" unanswerable at the audit layer for the operation that matters most:ALTER TABLE t CREATE BRANCH badds a ref at the current head and commits no snapshot, somainandbpoint at the same snapshot and are indistinguishable in the resulting state.Changes
jsonMetadataUpdatesfield onIcebergSnapshotsRequestBody. Purely additive: the server still builds table metadata fromjsonSnapshots/snapshotRefs, older clients omit the field, and consumers must tolerate null/empty. No existing field changes meaning.TableAuditEvent.branchRefName, populated from the commit'sset-snapshot-refaction.Why the Iceberg REST spec shape
The spec already models this exactly.
CommitTableRequestis{requirements[], updates[]}, whereupdates[]is a discriminated union onaction.CREATE BRANCH bis a singleset-snapshot-refwithref-name: b,type: branch, and noadd-snapshot.The client already has this list for free —
TableMetadata.changes(), the same one every Iceberg REST catalog sends (RESTTableOperationsline 136) — andMetadataUpdateParserserializes it in the spec wire format verbatim. So this ships the spec shape rather than a lookalike: when OpenHouse adopts the REST commit endpoint, the field is promoted toupdatesat the top level and the full-state fields retire, with no re-modeling.This is phase 1 of 3. Phase 2 has the server prefer these deltas over the snapshot-set diff it currently reconstructs in
OpenHouseInternalTableOperations(lines ~325-351, which already computes new-vs-existing snapshot IDs and diffs refs). Phase 3 replacesbaseTableVersionwith specrequirements[]. Each is separately reviewable; this PR changes no commit behavior.Why not infer it from the resulting state
The obvious approach — match the last snapshot in
jsonSnapshotsto the ref pointing at it — is wrong in four ways, all of whichCREATE BRANCHtriggers:CREATE BRANCH b,mainandbboth point at the last snapshot.snapshotRefsis aHashMap, so the answer depended on iteration order.SnapshotRefParserparses both and ref type was absent from the comparison.TableMetadata.snapshots(), which no API contract guarantees.Reading the commit's declared
set-snapshot-refremoves all four: no tie to break, no ordering assumption, and tags excluded by theirtype.Safety
branchRefNameis then left unset rather than guessed. An absent audit field is preferable to one that is wrong roughly half the time on ties.srcDirs), so both are covered.Testing Done
Server-side (
IcebergSnapshotsApiHandlerAuditTest), covering the cases the heuristic could not get right:CreateBranchAtHeadReportsNewBranchNotMain— the tie; reportsb, notmainCreateBranchIsDeterministicRegardlessOfRefOrder—LinkedHashMapwithmainfirst, still correctTagCommitLeavesBranchRefNameNull—type: tagignoredDropBranchLeavesBranchRefNameNull—remove-snapshot-refis not a writeWithoutMetadataUpdatesLeavesBranchRefNameNull— clients omitting the fieldSkipsUnparseableMetadataUpdate— malformed action does not hide valid onesClient-side (
OpenHouseTableOperationsMetadataUpdatesTest) assertschanges()produces the expected spec actions per operation: CREATE BRANCH emits only a branch-typedset-snapshot-refwith noadd-snapshot, CREATE TAG is tag-typed, DROP BRANCH is aremove-snapshot-ref, and an append reports bothadd-snapshotand the ref that moved.Note for reviewers:
TableMetadata.changes()accumulates across builds within a session, so the client fixture discards construction history to match production, where the base always comes from a refresh parsed off disk. This is the assumption the approach rests on.Additional Information
jsonMetadataUpdates, no consumer) and the audit consumer. Phases 2-3 described above are separate PRs.Not covered, and worth flagging: no end-to-end assertion through
BranchTestSpark3_5.java(which already has ~30CREATE BRANCHinvocations, including tie cases), andputSnapshotsForReplace(CTAS/RTAS) is not covered by thechanges()tests. Both are worth adding before phase 2 relies on this field for commit behavior rather than audit only.